Make registries distinguish between 'baseline is broken' and 'baseline missing'. #1203

BillyONeal · 2023-09-19T00:18:17Z

Next in the series containing #1153

…e missing'.

…s-error # Conflicts: # src/vcpkg/commands.ci-verify-versions.cpp # src/vcpkg/registries.cpp

include/vcpkg/paragraphs.h

include/vcpkg/registries.h

src/vcpkg/sourceparagraph.cpp

include/vcpkg/sourceparagraph.h

include/vcpkg/paragraphparser.h

include/vcpkg/sourceparagraph.h

…s-error

src/vcpkg/paragraphs.cpp

ras0219-msft · 2023-10-02T23:32:59Z

src/vcpkg/paragraphs.cpp

        ExpectedL<std::vector<Paragraph>> pghs = parse_paragraphs(StringView{text}, origin);
        if (auto vector_pghs = pghs.get())
        {
-            return SourceControlFile::parse_control_file(origin, std::move(*vector_pghs));
+            return map_parse_expected_to_localized_string(
+                SourceControlFile::parse_control_file(origin, std::move(*vector_pghs)));
        }

-        return ParseControlErrorInfo::from_error(origin, std::move(pghs).error());
+        return std::move(pghs).error();


suggestion:

return parse_paragraphs(StringView{text}, origin) .then([origin] (std::vector<Paragraph>&& p) { return map_parse_expected_to_localized_string( SourceControlFile::parse_control_file(origin, std::move(p))); });

See also my comment at the top about a more "functional" approach to map_parse_expected_to_localized_string, which would result in:

return parse_paragraphs(StringView{text}, origin) .then([origin] (std::vector<Paragraph>&& p) { return SourceControlFile::parse_control_file(origin, std::move(p)) .map_error(ToLocalizedString); });

Hmmmm I'm not a huge fan of nested transforms like that (map_error inside then) but this one might work.

I did the map_error but I'm opposed to replacing 1 if with double nested lambdas.

ras0219-msft · 2023-10-02T23:35:59Z

include/vcpkg/paragraphparser.h

@@ -49,11 +49,11 @@ namespace vcpkg
    using ParseExpected = vcpkg::ExpectedT<std::unique_ptr<P>, std::unique_ptr<ParseControlErrorInfo>>;

    template<class P>


suggestion:

static constexpr struct ToLocalizedString_t { LocalizedString operator()(std::unique_ptr<ParseControlErrorInfo> p) const; } ToLocalizedString;

Then, call sites can use this as

return maybe_parse_thing().map_error(ToLocalizedString);

ras0219-msft · 2023-10-02T23:41:01Z

src/vcpkg/paragraphs.cpp

-                return ParseControlErrorInfo::from_error(port_name, std::move(formatted));
+                return msg::format_error(msgFailedToParseManifest, msg::path = manifest_path)
+                    .append_raw("\n")
+                    .append(format_filesystem_call_error(ec, "read_contents", {manifest_path}));
            }

            if (fs.exists(control_path, IgnoreErrors{}))
            {
                ExpectedL<std::vector<Paragraph>> pghs = get_paragraphs(fs, control_path);


Similar to other suggestions, this could be:

return get_paragraphs(fs, control_path) .then([&control_path] (std::vector<Paragraph>&& p) { return SourceControlFile::parse_control_file(control_path, std::move(p)) .map_error(ToLocalizedString); });

src/vcpkg/paragraphs.cpp

src/vcpkg/portfileprovider.cpp

ras0219-msft · 2023-10-02T23:59:20Z

src/vcpkg/portfileprovider.cpp

+                    auto version = maybe_version->get();
+                    if (!version)
+                    {
+                        return msg::format_error(msgPortNotInBaseline, msg::package_name = port_name);


Should this also record the registry used for the lookup?

I looked at doing this but I'm not sure what we would print; registries don't have names and we don't currently have a way in console output to refer to a particular one. I think it would be good to emit the error attributed to the JSON document declaring the registry someday but I think that's outside the scope of this PR.

ras0219-msft · 2023-10-03T00:03:07Z

src/vcpkg/registries.cpp


    private:
        const ReadOnlyFilesystem& m_fs;

        Path m_path;
        std::string m_baseline_identifier;
-        DelayedInit<Baseline> m_baseline;
+        DelayedInit<ExpectedL<Optional<Baseline>>> m_baseline;


Shouldn't this be just ExpectedL<Baseline>?

src/vcpkg/registries.cpp

ras0219-msft · 2023-10-03T00:24:01Z

src/vcpkg/registries.cpp

-            auto b = load_baseline_versions(m_paths.get_filesystem(), *maybe_path.get()).value_or_exit(VCPKG_LINE_INFO);
-            if (auto p = b.get())
+
+            auto maybe_maybe_baseline = load_baseline_versions(m_paths.get_filesystem(), *path);


Are there any users that care about the ExpectedL<>(nullopt) case? This seems ripe for simplification to just ExpectedL<Baseline>.

I think consumers like BuiltinGitRegistry should be adding their context information (m_baseline_identifier) always -- why should "file not found" be special compared to "parse error"?

ExpectedL<>(nullopt) means 'that port is not in the baseline': https://github.com/microsoft/vcpkg-tool/pull/1203/files/38f738d8100a9119d08bf640fca4d5dceaef15ee#diff-9439a305925c63597b93bb0d49db1794b5b6120f7ca5456d3e99b403c862dbf5R93-R96

For instance, load_all_control_files ignores ports not in the baseline, but emits an error if there are errors:

https://github.com/microsoft/vcpkg-tool/pull/1203/files/38f738d8100a9119d08bf640fca4d5dceaef15ee#diff-d4fe92b1ebc56fd05568a18c509672434c578cf4e147b2d5aad36226c026ff00R547-R549

Both 'file not found' and 'parse failure' are errors. But 'parse success and the port you asked for is not in there' is not an error.

edit: I realized that my comment is in a poor location -- I'm talking about load_baseline_versions (called on this line), not get_baseline_version (the encapsulating function).

By my reading, this call is not "Get the baseline for a port" -- this is "Load the baseline file".

ExpectedT<Optional<Baseline>> load_baseline_versions(fs, path);

Agreed that for ExpectedT<Optional<Version>>, nullopt means "not in baseline". But for this function (load_baseline_versions), as far as I can tell nullopt means "The baseline.json didn't exist at all" -- which seems the same as a parse error.

Ah, I see. Thanks GH for making a totally reasonable comment opaque.

It looks like this was here because some callers emitted different error messages for not exist vs. parse error. I think, in general, sometimes this can make sense; I certainly used it for load_git_versions_file in x-ci-verify-versions to do something different for 'the versions file is corrupted' vs. 'this port does not have a versions file yet'.

However, I can't think of obvious reasons that would want to happen for baseline things, so I've ripped this one out.

ras0219-msft · 2023-10-05T19:18:40Z

src/vcpkg/paragraphs.cpp

+        auto pghs = parse_paragraphs(StringView{text}, origin);
        if (auto vector_pghs = pghs.get())
        {
-            return SourceControlFile::parse_control_file(origin, std::move(*vector_pghs));
+            return SourceControlFile::parse_control_file(origin, std::move(*vector_pghs)).map_error(ToLocalizedString);
        }

-        return ParseControlErrorInfo::from_error(origin, std::move(pghs).error());
+        return std::move(pghs).error();


Suggested change

auto pghs = parse_paragraphs(StringView{text}, origin);

if (auto vector_pghs = pghs.get())

{

return SourceControlFile::parse_control_file(origin, std::move(*vector_pghs));

return SourceControlFile::parse_control_file(origin, std::move(*vector_pghs)).map_error(ToLocalizedString);

}

return ParseControlErrorInfo::from_error(origin, std::move(pghs).error());

return std::move(pghs).error();

return parse_paragraphs(text, origin).then([origin] (auto&& p) {

return SourceControlFile::parse_control_file(origin, std::move(p)).map_error(ToLocalizedString);

});

ras0219-msft · 2023-10-05T19:30:44Z

src/vcpkg/registries.cpp

+                .map_error([&](LocalizedString&& error) {
+                    return std::move(error).append(msgWhileCheckingOutBaseline,
+                                                   msg::commit_sha = m_baseline_identifier);
+                });
        });

        auto baseline = maybe_baseline.get();


Below this line, in return maybe_baseline.error();, we should probably add at least some context of "while looking up port ". That at least will let users figure out the problematic registry themselves by matching patterns.

ras0219-msft · 2023-10-05T19:33:48Z

src/vcpkg/registries.cpp


-        return Optional<Version>();
+                return Optional<Version>();
+            });


Suggested change

});

}).map_error([port_name](LocalizedString&& err) {

return std::move(err).append("while finding a baseline for port {port_name}", port_name);

});

(approximately; I think reversing might be better)

I don't think map/then is clearer when processing needs to happen to 'both arms'. I did extract a common function though rather than repeating.

ras0219-msft · 2023-10-06T19:30:36Z

(Still happy with the merge of this PR, these three were just nits that I had queued up)

…soft#1203 after merge

BillyONeal closed this Sep 19, 2023

BillyONeal force-pushed the clarify-no-baseline-vs-error branch from 97651a5 to bfdd29e Compare September 19, 2023 00:21

Make registries distinguish between 'baseline is broken' and 'baselin…

e7a6683

…e missing'.

BillyONeal reopened this Sep 19, 2023

BillyONeal added 3 commits September 18, 2023 18:04

Fix move on constant variable.

09e135c

Fix merge conflict error.

4d8ca9e

Merge remote-tracking branch 'origin/main' into clarify-no-baseline-v…

ac43653

…s-error # Conflicts: # src/vcpkg/commands.ci-verify-versions.cpp # src/vcpkg/registries.cpp

BillyONeal commented Sep 22, 2023

View reviewed changes

include/vcpkg/paragraphs.h Outdated Show resolved Hide resolved

BillyONeal mentioned this pull request Sep 23, 2023

Teach x-ci-verify-versions to check that versions exist in the database #1210

Merged

ras0219-msft reviewed Sep 28, 2023

View reviewed changes

BillyONeal added 2 commits September 28, 2023 17:19

Merge remote-tracking branch 'origin/main' into clarify-no-baseline-v…

43c98e5

…s-error

Code review feedback

38f738d

BillyONeal mentioned this pull request Sep 29, 2023

Fix crash while parsing malformed manifest files. #1211

Closed

ras0219-msft approved these changes Oct 3, 2023

View reviewed changes

CR feedback from @ras0219-msft

6ca505e

BillyONeal requested a review from ras0219-msft October 5, 2023 05:45

BillyONeal merged commit 0d3da08 into microsoft:main Oct 6, 2023
5 checks passed

BillyONeal deleted the clarify-no-baseline-vs-error branch October 6, 2023 03:22

ras0219-msft reviewed Oct 6, 2023

View reviewed changes

BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Oct 10, 2023

Collapse out a few more maybe_maybes and respond to comments of micro…

70e914b

…soft#1203 after merge

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make registries distinguish between 'baseline is broken' and 'baseline missing'. #1203

Make registries distinguish between 'baseline is broken' and 'baseline missing'. #1203

BillyONeal commented Sep 19, 2023

ras0219-msft Oct 2, 2023

BillyONeal Oct 3, 2023

BillyONeal Oct 5, 2023

ras0219-msft Oct 2, 2023

BillyONeal Oct 5, 2023

ras0219-msft Oct 2, 2023

ras0219-msft Oct 2, 2023

BillyONeal Oct 5, 2023

ras0219-msft Oct 3, 2023

ras0219-msft Oct 3, 2023

BillyONeal Oct 3, 2023

ras0219-msft Oct 3, 2023 •

edited

Loading

BillyONeal Oct 5, 2023

ras0219-msft Oct 5, 2023

ras0219-msft Oct 5, 2023

ras0219-msft Oct 5, 2023

BillyONeal Oct 10, 2023

ras0219-msft commented Oct 6, 2023

		@@ -49,11 +49,11 @@ namespace vcpkg
		using ParseExpected = vcpkg::ExpectedT<std::unique_ptr<P>, std::unique_ptr<ParseControlErrorInfo>>;

		template<class P>

-            });
+            }).map_error([port_name](LocalizedString&& err) {
+                return std::move(err).append("while finding a baseline for port {port_name}", port_name);
+            });

Make registries distinguish between 'baseline is broken' and 'baseline missing'. #1203

Make registries distinguish between 'baseline is broken' and 'baseline missing'. #1203

Conversation

BillyONeal commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ras0219-msft Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ras0219-msft commented Oct 6, 2023

ras0219-msft Oct 3, 2023 •

edited

Loading